Feature/performant old filmlist reader #150

Nicklas2751 · 2022-02-06T17:30:25Z

In context of the Filmlistmerger I created a new, more performant, reader for the old Filmlist .

…& mapstruct

sonarcloud · 2022-02-06T17:49:10Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

TheSasch

Didn't had the time to have a look at the unit tests. I will do it later.

pom.xml

TheSasch · 2022-02-07T07:57:57Z

pom.xml

@@ -352,7 +337,7 @@
          <plugin>
            <groupId>org.sonatype.plugins</groupId>
            <artifactId>nexus-staging-maven-plugin</artifactId>
-            <version>${nexus-staging-maven-plugin.version}</version>
+            <version>1.6.8</version>


Can't do it for the plugins inside the profile. When I do it the version is missing.

TheSasch · 2022-02-07T07:58:09Z

pom.xml

@@ -363,7 +348,7 @@
          <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-gpg-plugin</artifactId>
-            <version>${maven-gpg-plugin.version}</version>
+            <version>3.0.1</version>


Same as the one before, its inside the profile so I can't do

TheSasch · 2022-02-07T07:59:52Z

src/main/java/de/mediathekview/mlib/filmlisten/reader/FilmlistOldFormatReader.java

-    } catch (final Exception exception) {
-      LOG.debug(
-          String.format("Error on converting the following text to a film:%n %s ", splittedEntry));
+      return Optional.of(RawFilmToFilmMapper.INSTANCE.rawFilmToFilm(rawFilm, rawFilm));


You should use DI from Spring instead of using the INSTANCE variable here. Please inject the mapper and just use it. It is possible that you have to define the generating component model within your mapper (https://mapstruct.org/documentation/stable/reference/html/#using-dependency-injection)

Its the MLib which isn't a spring application. I just use the spring boot parent for the dependency versions.

TheSasch · 2022-02-07T08:11:39Z

src/main/java/de/mediathekview/mlib/filmlisten/reader/OldFilmlistToRawFilmlistReader.java

+
+  private UUID toListId(String entryPart) {
+    try {
+      String rawUUID = entryPart.replaceFirst("\"].*", "");


Define a constant with the regex. If you want let's talk about that method.

TheSasch · 2022-02-07T08:14:44Z

src/main/java/de/mediathekview/mlib/filmlisten/reader/OldFilmlistToRawFilmlistReader.java

+        LocalTime.parse(dateTimeSplitted[1], TIME_FORMATTER));
+  }
+
+  private String clearField(String rawField) {


A whole method that could be a regex

TheSasch · 2022-02-07T08:17:44Z

src/main/java/de/mediathekview/mlib/filmlisten/reader/RawFilmToFilmMapper.java

+import static java.time.format.FormatStyle.MEDIUM;
+
+@Mapper
+public interface RawFilmToFilmMapper {


Ok, lets talk about the mapper :)

pidoubleyou · 2022-02-09T20:29:34Z

pom.xml

@@ -1,6 +1,13 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
  <modelVersion>4.0.0</modelVersion>
+  <parent>
+    <groupId>org.springframework.boot</groupId>
+    <artifactId>spring-boot-starter-parent</artifactId>


wieso Spring Boot? wird doch in MLib gar nicht verwendet.

Im spring Boot parent sind für viele Abhängigkeiten die Versionen aufeinander abgestimmt und getestet. Wenn wir den, wie hier von mir getan, nutzen müssen wir uns nicht um Kompatibilität einzelner Versionen zueinander kümmern. Zum updaten von dependencies können wir so auch "einfach" die Spring version erhöhen und müssen uns dann nur noch um die zusätzlichen Deps kümmern.

pidoubleyou · 2022-02-09T20:33:36Z

src/main/java/de/mediathekview/mlib/filmlisten/reader/OldFilmlistToRawFilmlistReader.java

+        LocalTime.parse(dateTimeSplitted[1], TIME_FORMATTER));
+  }
+
+  private String clearField(String rawField) {


Naming: was bereinigt die Methode?

Guter Hinweis

src/main/java/de/mediathekview/mlib/filmlisten/reader/RawFilm.java

pidoubleyou · 2022-02-09T20:39:10Z

src/main/java/de/mediathekview/mlib/tool/FileSizeDeterminer.java

@@ -9,7 +9,7 @@

 import java.io.IOException;

-import static jakarta.ws.rs.core.HttpHeaders.CONTENT_LENGTH;
+import static javax.ws.rs.core.HttpHeaders.CONTENT_LENGTH;


MServer verwendet die jakarta-Variante, keine Ahnung ob das im Zusammenspiel mit MLib problematisch werden könnte. Warum wieder zurück zur "alten" Variante?

Dependency Version hat sich geändert und da sind die Pfade noch so

…learField naming

TheSasch

I also wrote some findings to Discord.

TheSasch · 2022-02-27T19:53:20Z

src/main/java/de/mediathekview/mlib/filmlisten/reader/RawFilmToFilmMapper.java

+
+@Mapper
+public interface RawFilmToFilmMapper {
+  Logger LOG = LogManager.getLogger(RawFilmToFilmMapper.class);


Use @slf4j when you already use Lombok instead of self definiton

TheSasch · 2022-02-27T20:02:01Z

src/main/java/de/mediathekview/mlib/filmlisten/reader/RawFilmToFilmMapper.java

+  Film rawFilmToFilm(RawFilm rawFilm);
+
+  default List<GeoLocations> mapGeolocation(RawFilm rawFilm) {
+    return GeoLocations.find(rawFilm.getGeo()).map(List::of).orElse(new ArrayList<>());


return GeoLocations.find(rawFilm.getGeo()).stream().collect(Collectors.toList());

TheSasch · 2022-02-27T20:03:41Z

src/main/java/de/mediathekview/mlib/filmlisten/reader/RawFilmToFilmMapper.java

+
+  @AfterMapping
+  default void complexMappings(RawFilm rawFilm, @MappingTarget Film film) {
+    long groesse = mapSize(rawFilm);


could be final

sonarcloud · 2022-03-09T18:11:35Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
21 Code Smells

0.0% Coverage
0.0% Duplication

sonarcloud · 2022-03-09T18:11:35Z

Please retry analysis of this Pull-Request directly on SonarCloud.

Nicklas2751 added 3 commits February 6, 2022 18:24

Add spring parent pom, cleanup dependencies, update deps, add lombok …

191dd74

…& mapstruct

Add a more performant old filmlist format reader

1558bc1

Fix a maven pom waring with empty xml element

49b3861

Nicklas2751 requested review from pidoubleyou and TheSasch February 6, 2022 17:30

Nicklas2751 added 2 commits February 6, 2022 18:37

Fix missing plugin version

5977f74

Fix code smells

2008975

TheSasch reviewed Feb 7, 2022

View reviewed changes

pidoubleyou reviewed Feb 9, 2022

View reviewed changes

Nicklas2751 added 4 commits February 19, 2022 11:52

Move plugin versions to vars, Remove unused RawFilm fields & change c…

8f475fe

…learField naming

Change entry delimiter pattern because it may contain whitespace

afc6fe9

Add missing test for raw film to film mapper

0a6298b

Refactor raw film to film mapper

e90f29a

Nicklas2751 requested a review from TheSasch February 27, 2022 16:01

TheSasch reviewed Feb 27, 2022

View reviewed changes

TheSasch and others added 4 commits March 5, 2022 16:56

Don't know if it works but here is our pairing

6901d68

Remove mock & add more tests

766e558

Add more mapper tests

f6dfac0

Fix small mapper bugs & add more mapper tests

a1ad4df

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/performant old filmlist reader #150

Feature/performant old filmlist reader #150

Nicklas2751 commented Feb 6, 2022

sonarcloud bot commented Feb 6, 2022

TheSasch left a comment

TheSasch Feb 7, 2022

Nicklas2751 Feb 19, 2022

TheSasch Feb 7, 2022

Nicklas2751 Feb 19, 2022

TheSasch Feb 7, 2022

Nicklas2751 Feb 19, 2022

TheSasch Feb 7, 2022

TheSasch Feb 7, 2022

TheSasch Feb 7, 2022

Nicklas2751 Feb 19, 2022

pidoubleyou Feb 9, 2022

Nicklas2751 Feb 19, 2022

pidoubleyou Feb 9, 2022

Nicklas2751 Feb 19, 2022

pidoubleyou Feb 9, 2022

Nicklas2751 Feb 19, 2022

TheSasch left a comment

TheSasch Feb 27, 2022

TheSasch Feb 27, 2022

TheSasch Feb 27, 2022

sonarcloud bot commented Mar 9, 2022

sonarcloud bot commented Mar 9, 2022

Feature/performant old filmlist reader #150

Are you sure you want to change the base?

Feature/performant old filmlist reader #150

Conversation

Nicklas2751 commented Feb 6, 2022

sonarcloud bot commented Feb 6, 2022

TheSasch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheSasch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 9, 2022

sonarcloud bot commented Mar 9, 2022